capnproto / capnpc-rust

Cap'n Proto code generation for Rust
76 stars 26 forks source link

support Cap'n Proto generics #18

Closed kali closed 9 years ago

kali commented 9 years ago

What I'm referring to is the fact that Map(Key,Value) is translated to a pair of unparameterized rust type (one builder, one reader), using "any_pointer". It makes it very easy to generate invalid data. I think it would be much nicer to parameterize the Map Builder and Reader so that the right Builder and Reader. It would probably enhance the API safety, and client code readability.

I suspect this is by-design, but I find it very disturbing when working in rust with its very strict typesystem.

dwrensha commented 9 years ago

Hi! It sounds like you are using generics. Unfortunately, these are not yet fully supported in capnproto-rust. You can use them, but the type information will be lost, as you've noticed.

Fully implementing generics will take a bit of work. The groundwork is in place with the traits that are defined in ::capnp::traits. I can give you some guidance if you're interested in helping out on it. :)

kali commented 9 years ago

All right, I'm giving it a shot. My understanding so far is that the runtime part is more or less done, what remains is mostly codegen stuff.

So I started playing codegenerator-by-hand in order to get a grasp of how everything is working :

struct One(T) {   it @0: T; }
struct Alpha { foo @0: One(Text); }

I cut and paste the generated code, and tried to patch it to get a type-safe One::get_it() as a starter. I got this to compile:

pub mod one {
  /* capnp imports */
  use std::marker::PhantomData;

  #[derive(Clone, Copy)]
  pub struct Reader<'a,TR>
        where TR: FromPointerReader<'a> {
    reader : layout::StructReader<'a>,
    _it: PhantomData<TR>
  }

  impl <'a, TR> ::capnp::traits::HasTypeId for Reader<'a,TR>
        where TR: FromPointerReader<'a> {
    #[inline]
    fn type_id() -> u64 { _private::TYPE_ID }
  }
  impl <'a, TR> ::capnp::traits::FromStructReader<'a> for Reader<'a,TR>
        where TR: FromPointerReader<'a> {
    fn new(reader: ::capnp::private::layout::StructReader<'a>) -> Reader<'a,TR> {
      Reader { reader : reader, _it: PhantomData }
    }
  }

  impl <'a, TR> ::capnp::traits::FromPointerReader<'a> for Reader<'a,TR>
        where TR: FromPointerReader<'a> {
    fn get_from_pointer(reader: &::capnp::private::layout::PointerReader<'a>) -> Result<Reader<'a,TR>> {
      ::std::result::Result::Ok(::capnp::traits::FromStructReader::new(try!(reader.get_struct(::std::ptr::null()))))
    }
  }

  impl <'a, 'b : 'a, TRa,TRb> ::capnp::traits::CastableTo<Reader<'a,TRa>> for Reader<'b,TRb>
        where TRa: FromPointerReader<'a>, TRb: FromPointerReader<'b> {
    fn cast(self) -> Reader<'a,TRa> { Reader { reader : self.reader, _it: PhantomData } }
  }

  impl <'a, TR> Reader<'a,TR>
        where TR: FromPointerReader<'a> {
    pub fn borrow<'b>(&'b self) -> Reader<'b,TR> {
      Reader { reader : self.reader, _it: PhantomData }
    }

    pub fn total_size(&self) -> Result<::capnp::MessageSize> {
      self.reader.total_size()
    }

    #[inline]
    pub fn get_it(self) -> Result<TR> {
      TR::get_from_pointer(&self.reader.get_pointer_field(0))
    }

    pub fn has_it(&self) -> bool {
      !self.reader.get_pointer_field(0).is_null()
    }
  }

alpha::Reader::get_foo() has to be patched too:

    #[inline]
    pub fn get_foo(self) -> Result<::schema_capnp::one::Reader<'a, text::Reader<'a>>> {
      ::capnp::traits::FromPointerReader::get_from_pointer(&self.reader.get_pointer_field(0))
    }

Am I on the right tracks so far ?

kali commented 9 years ago

I got the builder to compile too. But it's not usable as is because the specific setters methods for Text and Data (and maybe others) can not be generated in a generic fashion. Do we need to augment TextBuilder and DataBuilder to support a use case where GenericStruct::Builder<...>.get_[text_field_name]() return a TextBuilder that can reallocate itself?

dwrensha commented 9 years ago

I think a generic setter function would look something like this:

  impl <'a, TB> Builder<'a, TB> {
    pub fn set_foo<TR>(&mut self, value: TR) where TR: SetPointerBuilder<TB> {
        SetPointerBuilder::set_pointer_builder(builder.get_pointer_field(0), value);
    }
  }
kali commented 9 years ago

Indeed, it does. Thanks for the hint. I'll start integrating this stuff in the code generator in few days and see what's missing at that point.

kali commented 9 years ago

Hey, I started hacking the code generator, but I think there is something I don't get.

I am trying to compile:

struct GenericStruct(Foo,Bar) {
    fooField @0 : Foo;
    barField @1 : Bar;
}

But here is what the generator receives from capnp:

( nodes = [
    ( id = 18063754139534309017,
      displayName = "test.capnp:GenericStruct",
      displayNamePrefixLength = 11,
      scopeId = 11083788732080705255,
      nestedNodes = [],
      struct = (
        dataWordCount = 0,
        pointerCount = 2,
        preferredListEncoding = inlineComposite,
        isGroup = false,
        discriminantCount = 0,
        discriminantOffset = 0,
        fields = [
          ( name = "fooField",
            codeOrder = 0,
            discriminantValue = 65535,
            slot = (
              offset = 0,
              type = (
                anyPointer = (
                  unconstrained = (anyKind = void) ) ),
              defaultValue = (
                anyPointer = <opaque pointer> ),
              hadExplicitDefault = false ),
            ordinal = (explicit = 0) ),
          ( name = "barField",
            codeOrder = 1,
            discriminantValue = 65535,
            slot = (
              offset = 1,
              type = (
                anyPointer = (
                  unconstrained = (anyKind = void) ) ),
              defaultValue = (
                anyPointer = <opaque pointer> ),
              hadExplicitDefault = false ),
            ordinal = (explicit = 1) ) ] ),
      parameters = [(name = "Foo"), (name = "Bar")],
      isGeneric = true ),

So there is no clear indication of the actual type for the two fields being constrained, even if the struct appears parameterized. What am I missing ?

dwrensha commented 9 years ago

That's odd. Which version of capnp are you using? How exactly are you generating that output?

Here's what I see when I try:

# test.capnp
@0xdee3be1e23089387;

struct GenericStruct(Foo,Bar) {
    fooField @0 : Foo;
    barField @1 : Bar;
}
$ capnp --version
Cap'n Proto version 0.6-dev
$ capnp compile -o- test.capnp | capnp decode /usr/local/include/capnp/schema.capnp CodeGeneratorRequest
( nodes = [
    ( id = 16986253768312819274,
      displayName = "test.capnp:GenericStruct",
      displayNamePrefixLength = 11,
      scopeId = 16060889732779381639,
      nestedNodes = [],
      struct = (
        dataWordCount = 0,
        pointerCount = 2,
        preferredListEncoding = inlineComposite,
        isGroup = false,
        discriminantCount = 0,
        discriminantOffset = 0,
        fields = [
          ( name = "fooField",
            codeOrder = 0,
            discriminantValue = 65535,
            slot = (
              offset = 0,
              type = (
                anyPointer = (
                  parameter = (
                    scopeId = 16986253768312819274,
                    parameterIndex = 0 ) ) ),
              defaultValue = (
                anyPointer = <opaque pointer> ),
              hadExplicitDefault = false ),
            ordinal = (explicit = 0) ),
          ( name = "barField",
            codeOrder = 1,
            discriminantValue = 65535,
            slot = (
              offset = 1,
              type = (
                anyPointer = (
                  parameter = (
                    scopeId = 16986253768312819274,
                    parameterIndex = 1 ) ) ),
              defaultValue = (
                anyPointer = <opaque pointer> ),
              hadExplicitDefault = false ),
            ordinal = (explicit = 1) ) ] ),
      parameters = [(name = "Foo"), (name = "Bar")],
      isGeneric = true ),
    ( id = 16060889732779381639,
      displayName = "test.capnp",
      displayNamePrefixLength = 5,
      scopeId = 0,
      nestedNodes = [
        ( name = "GenericStruct",
          id = 16986253768312819274 ) ],
      file = void,
      isGeneric = false ) ],
  requestedFiles = [
    ( id = 16060889732779381639,
      filename = "test.capnp",
      imports = [] ) ] )
dwrensha commented 9 years ago

(Note that although I'm using the 0.6 dev branch, this should also work with Cap'n Proto 0.5.x)

kali commented 9 years ago

OK. Using capnp tools 0.5.2 from brew with the schema from /usr/local/..., I do get the parameterized type in the output. The problem appears when I am decoding its output using the schema.capnp at the top level of the capnpc-rust repository. As far as I can tell, it is a more recent version, and the changes are about right where anypointer type enum is defined. Is this incompatibility expected ? Any hint on how the rust generator should deal with that ?

dwrensha commented 9 years ago

... schema.capnp at the top level of the capnpc-rust repository.

There should not be a "schema.capnp" file in the repo. Maybe you copied it from elsewhere?

kali commented 9 years ago

Oups... I certainly did at some point, trying to understand what was happening, and got side-tracked.

Anyway, the heart of my problem is as follow: If I instrument getter_text any_pointer section like that:

        Ok(field::Slot(reg_field)) => {
               println!("XXXX {:?}", field.get_name());
[...]
                Ok((type_::AnyPointer(pointer), _)) => {
                    match pointer.which().unwrap() {
                        type_::any_pointer::Parameter(param) => println!("PARAM"),
                        type_::any_pointer::Unconstrained(unco) => println!("UNCO"),
                        type_::any_pointer::ImplicitMethodParameter(param) => println!("IMPLICIT"),
                    }

                    return (format!("::capnp::any_pointer::{}<{}>", module, lifetime),
                            Line(format!("::capnp::any_pointer::{}::new(self.{}.get_pointer_field({}))",
                                         module, member, offset)))

                }

All fields are detected as unconstrained, including the ones I parameterized. I noticed that the schema changes between 0.5.2 and the current 0.6 are specifically impacting the any_pointer union. Finally I'm a bit puzzled by the "2" in https://github.com/dwrensha/capnpc-rust/blob/master/src/schema_capnp.rs#L3574 , which seems particularly arbitrary to me (but I haven't studied the schema compiler in depth.).

dwrensha commented 9 years ago

Eek. Good find! It looks like this is a bug in the Cap'n Proto schema compiler. I've reported it here: https://github.com/sandstorm-io/capnproto/issues/219

kali commented 9 years ago

ok. i'll setup 0.6.

dwrensha commented 9 years ago

The fixes are in. You should be able to use Cap'n Proto 0.5.x with https://github.com/dwrensha/capnpc-rust/commit/6e9d657cc338cc0d0183265cd77a015a0afcae3b.

dwrensha commented 9 years ago

With #20 we now have preliminary support for generics. Let's keep this issue open until we have full support.

One thing that currently doesn't work:

struct A(T) { struct B { t @0 :T; } }
dwrensha commented 9 years ago

With #23 and my work from this weekend, we are now most of the way there!

One issue that remains: if we have

struct Foo(T) { foo @0 :T; }

and we instantiate e.g. a Foo(List(u8)) in Rust code, then we have no way to initialize the foo field with a given size. I think we should generate an additional method foo::Builder::init_sized_foo(u32) in addition to the usual foo::Builder::init_foo() method.

dwrensha commented 9 years ago

Okay, now we handle the case I was worried about in my previous comment with an additional method foo::Builder::initn_foo(u32).

Generics should be usable now.