Usage of newtypes instead of type aliases in generated code

[Cerber-Ursi]

I'm wondering whether it's possible to use the transparent newtypes everywhere the current generated code uses type aliases (e.g. Window.

For now, I can see only one problem - the fact that we have to generate ID first (in a general way), and only then give this ID a meaning by allocating the corresponding resource; this makes it probably infeasible to use the traditional newtype pattern, i.e. replace type Window = u32 with struct Window(u32). However, it might be possible to do the following (that's just a sketch, without thinking too much of ergonomics yet):

• First, add struct Id<Type>(u32, PhantomData<Type>) as a generic "handler type" (probably with #[repr(transparent)]).
• Second, set the return type of Connection::generate_id to be Id<()> (i.e. "handler to something").
• Third, add marker structs such as struct WindowH.
• Fourth, set type aliases to the different specifications of Id, e.g. type Window = Id<WindowH>.

This way, freshly generated IDs will not be usable, unless one explicitly states what they want to use them for. That might reduce the possible confusion, especially for functions with long argument lists.

If this approach can be useful, I'll start working on the PR. If not, it'd probably be good to have the reasons for such decision documented somewhere?

[psychon]

Just to summarize: You want to turn the following into a compiler error, right? (Problem is that a CreateWindow creates a window while FreePixmap expects a pixmap and will produce a runtime error)

let id = connection.create_id()?;
connection.create_window(id, other arguments)?;
connection.free_pixmap(id)?;

I didn't really think much about your proposed solution, but the above is the problem you want to solve. Is this something that "actually happens a lot" (did it happen to you)? Or is this just due to "it feels more Rust-y this way" (with which I certainly agree)?

If this approach can be useful, I'll start working on the PR.

How would you do that? The XML does not contain the necessary type information, I think.

Actually "I thought", but apparently it does since I just found:

  <xidunion name="DRAWABLE">
    <type>WINDOW</type>
    <type>PIXMAP</type>
  </xidunion>

So, apparently the necessary information is available (but I bet somewhere in the XML something got this wrong since libxcb does not use this information at all. xcbgen does not even parse the contents of an <xidunion> and there are only three instances of it in all the XML).

Third, add marker structs such as struct WindowH.

Why? Why not just have several newtypes?

#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] // Dunno if I got all traits
struct XId(u32); // This is what `generate_id()` produces
impl From<Xid> for u32 {...}
impl From<u32> for Xid {...}

#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
struct WindowId(u32);
impl From<WindowId> for u32 {...}
impl From<Xid> for WindowId {...}
impl From<u32> for WindowId {...}

#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
struct PixmapId(u32);
impl From<PixmapId> for u32 {...}
impl From<Xid> for PixmapId {...}
impl From<u32> for PixmapId {...}

#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
struct DrawableId(u32);
impl From<DrawableId> for u32 {...}
impl From<Xid> for DrawableId {...}
impl From<u32> for DrawableId {...}
impl From<WindowId> for DrawableId {...}
impl From<PixmapId> for DrawableId {...}

Hm... would the drawable argument of copy_area then become a generic Into<DrawableId>? That way, one could also use this with our WindowWrapper (if that implements the necessary trait).

Edit: Hm, all those Froms mean that this idea does not actually provide the wanted guarantees, because one can just keep the return value from generate_id() as an Xid without having to convert it to anything else...

[Cerber-Ursi]

You want to turn the following into a compiler error, right? (Problem is that a CreateWindow creates a window while FreePixmap expects a pixmap and will produce a runtime error)

Yes, that's correct.

Is this something that "actually happens a lot" (did it happen to you)? Or is this just due to "it feels more Rust-y this way" (with which I certainly agree)?

Just an attempt to make this API a little more idiomatic, nothing more. I haven't worked with the library yet, to be honest.

The XML does not contain the necessary type information, I think.

Not sure what do you mean here. I was thinking only of the currently generated type aliases, which, if I understand correctly, are created from the xidtype elements. The xidunion complicates this a bit, however, but seems to be doable nevertheless, if the definitions themselves are correct. Probably I'm missing something else?

Why not just have several newtypes?

That's more or less the same approach, but you're right, it seems to be simpler.

would the drawable argument of copy_area then become a generic Into? That way, one could also use this with our WindowWrapper (if that implements the necessary trait).

Or we generate the as_drawable_id on anything relevant. This might lead to a more verbose code, but the API will be a little simpler with less generics and implicit conversions.

Hm, all those Froms mean that this idea does not actually provide the wanted guarantees, because one can just keep the return value from generate_id() as an Xid without having to convert it to anything else...

Could it be a good idea then to not implement neither Copy nor Clone on Xid, so that one can only convert it into more specific identifier (which then will itself be copyable)? Like this (playground):

#[derive(Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
struct XId(u32);

#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
struct WindowId(u32);
impl From<XId> for WindowId {
    fn from(xid: XId) -> WindowId {
        WindowId(xid.0)
    }
}

#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
struct PixmapId(u32);
impl From<XId> for PixmapId {
    fn from(xid: XId) -> PixmapId {
        PixmapId(xid.0)
    }
}

fn generate_id() -> XId {
    XId(0) // just a stub
}

fn create_window(_: WindowId) {
    // an empty stub
}

fn free_pixmap(_: PixmapId) {
    // an empty stub
}

fn main() {
    let wid = generate_id().into();
    create_window(wid);
    // free_pixmap(wid); - error, mismatched types
    // we can then do something else with `wid` - it's Copy, so it's not gone
    
    let pid = generate_id().into();
    free_pixmap(pid);
    // create_window(pid); - error, mismatched types
    
    let xid = generate_id();
    create_window(xid.into());
    // free_pixmap(xid.into()); error, xid was moved
}

[notgull]

Hm... would the drawable argument of copy_area then become a generic Into?

This is actually how I did it in breadx. Each XID type was a wrapper around a u32 with From impls for u32 as well as anything it was a union of. I also used Into<DrawableId> for functions in breadx. The only real issue is that it generates two separate functions when you call it for, say, both Window and Drawable. However, considering that, in this alternate reality, Window and Drawable both refer to the same data, this would mean the exact same bytecode would be generated for each function. I think newer versions of rustc can optimize it so that only one of those functions get used (at the cost of some minor confusion during debugging).

Could it be a good idea then to not implement neither Copy nor Clone on Xid

I don't think so, since there are a million ways around it anyways.