Use custom derives for builders instead of Macros

[hollinwilkins]

Currently a system of macros is used to generate builders, which requires a large amount of generated code. Using custom derived traits for the builders instead would save how much generated code we need to produce and simplify creating builders.

[hollinwilkins]

@josephDunne I had a chance to look a bit into this yesterday, and I want to make sure I understand it before we make the change.

This is what we get out of the generator right now for a builder:

The builder trait.

/// Builder Trait for `Weapon` tables.
pub trait WeaponBuilder {
    fn start_weapon(&mut self);
    /// Set the value for field `name`.
    fn add_name(&mut self, name: flatbuffers::UOffsetT);
    /// Set the value for field `damage`.
    fn add_damage(&mut self, damage: i16);
}

The implementation of the builder trait.

impl WeaponBuilder for flatbuffers::Builder {
    fn start_weapon(&mut self) {
        self.start_object(2);
    }

    fn add_name(&mut self, name: flatbuffers::UOffsetT) {
        self.add_slot_uoffset(0, name, 0)
    }

    fn add_damage(&mut self, damage: i16) {
        self.add_slot_i16(1, damage, 0)
    }

}

The table object itself.

flatbuffers_object!{Table => Weapon [
 field => { name = name,
            typeOf = string,
            slot = 4,
            default = 0 }, 
 field => { name = damage,
            typeOf = i16,
            slot = 6,
            default = 0 }]}

Currently, builders are generated entirely in flatc, but it sounds like we want to move some of that code generation out of flatc and into a custom derived trait.

To be clear:

1. We want to continue to generate the table object with the macros we have right now
2. We want to continue to generate the WeaponBuilder trait using flatc
3. This is where I am unclear. In what way do we want to use custom derived traits?

[josephDunne]

@hollinwilkins I think we should drop the macros and switch to custom derive.
This is what flatc should output:

#[derive(Flatbuffer, FlatBufferBuilder)]
struct Weapon {
   name: String,
  damage: i16
}

The macro code is difficult to maintain but i did it because:

1. I do not want to maintain the c++ code so the c++ code should be small as possible
2. I want crate to be a stand alone rust crate so must be able to work without going through flatc.
   2.i) this can be achieved by having a build.rs that fetches and calls flatc
   2.ii) or it can be done with macros/custom derive
3. initialy when i wrote this procedural macros and custom derive where nightly only and the google team didn't want to accept a dependency on nightly (which is fair i guess).

The macro code is made more difficult by the fact that I can't do macro expansion in the Type position (although perhaps this is fixed now).

so we have two options:

1. improve the current macro system
   or
2. switch to custom derive

Custom derive is more ergonomic and more familiar to the rust community (because Serde uses them).
Also there is no need to remember the complicated macro format. Error messages will also be more informative.

thought?

[hollinwilkins]

@josephDunne This is definitely much cleaner. I haven't used Rust custom derives or macro system extensively, so I'll ask you a few implementation details here before I get started:

1. Are the macros generating some struct like this?

struct WeaponFlatBuffer<T: AsRef<[u8]>> {
  table: flatbuffers::Table<T>
}

2. Along with impls like this?

impl<T: AsRef<[u8]>> WeaponFlatBuffer<T> {
  fn get_name(&self) -> &str { ... }
  fn get_damage(&self) -> u16 { ... }
}

3. And finally, the macros are generation another struct/impl for the actual builder:

struct WeaponBuilder {
  fbb: flatbuffers::Builder
}

impl WeaponBuilder {
  fn start_weapon(&mut self) -> { ... }
  fn add_name(&mut self, name: flatbuffers::UOffsetT) -> { ... }
  fn add_damage(&mut self, damage: i16) -> { ... }
}

I know this is not including a lot of items, but I want to make sure I understand the capabilities/limits of custom derive and also the core set of impls/structs we are meant to be generating using macros/custom derive.

[josephDunne]

@hollinwilkins

1. Are the macros generating some struct like this?

yes

2. Along with impls like this?

yes. In addition to the structs and impls you outlined we could do something like this:

impl<T> From<WeaponFlatBuffer<T>> for Weapon {
...
}

naturally this goes against the whole point of flatbuffers but it could be useful. A more useful trait might be:

impl Into<WeaponFlatBuffer<Vec<u8>> for Weapon {

}

which uses a WeaponBuilder to construct a owned Table i.e. Table<Vec<u8> At the very least these are useful in writing tests.

3. And finally, the macros are generation another struct/impl for the actual builder:

yes.

On a separate note we could replace the struct Table<T> with a trait Table. Then we would have:

struct WeaponFlatBuffer<T: AsRef<[u8]>>(T)

impl<T> Table for WeaponFlatBuffer<T> 
     where T: AsRef<[u8]> {}

All the functions in the Table trait would have default implementations so the above impl would be all that is necessary to turn WeaponFlatBuffer into a Table.

This needs some thought so as you are looking into this let me know what you think. For now we can stick with what we have. Will be trivial to change later.

[hollinwilkins]

👍 This makes a lot of sense, it will be nice. I'll try to get a start on this work tonight and keep you posted as I find issues or questions come up

[hollinwilkins]

What do you think of making the base struct from which we derive this so we can reserve Weapon for the generated struct

#[derive(FlatBuffer, FlatBufferBuilder)]
struct WeaponSpec {
  name: String,
  damage: i16
}

[hollinwilkins]

@josephDunne I ended up creating a sample (compilable) test file to explore some ideas for the code these macros could produce. It ended up being pretty long because of all the comments I added, so I create a gist for it. Here it is, let me know what you think:

https://gist.github.com/hollinwilkins/7c9b8f0c642e5c53e1727e74133b29e2

[josephDunne]

What do you think of making the base struct from which we derive this so we can reserve Weapon for the generated struct

This would force the end user to remember to call the struct SomethingSpec. Also if you have an existing code base it would prevent you from just adding derive annotations to existing structs. So I'm against the idea.

[josephDunne]

 // Possible improvement, which would cause object
  // to be automatically closed on behalf of the user
  //
  // The idea here is to prevent misuse of the library
  pub fn build_weapon<F>(&mut self, f: F) -> flatbuffers::UOffsetT
    where F: FnOnce() -> () {
      self.builder.start_object(2);
      f();
      self.builder.end_object()
    }

I think we should go with the standard builder pattern as outlined here: https://aturon.github.io/ownership/builders.html and I think because we are populating a buffer we will have to be a consuming builder i.e. the function above would become:

 pub fn build(self) -> Vec<u8> {
    self.end_object();
    //return underlying buffer which could be a Vec<u8> if we own the buffer or a &mut Vec<u8> if we are being used to populate a vector owned by another builder
}

the all builder methods would take self and return self versus &mut self

then you could have:

WeaponBuilder::new()
                          .add_name("test")
                          .add_damage(16)
                          .build()

and

WeaponBuilder::new_with(&mut some_u8_vec)
                          .add_name("test")
                          .add_damage(16)
                          .build()

[josephDunne]

// Change AsRef<[u8]> to Deref<Target=[u8]> (maybe not) so
// that the underlying buffer storage can be an Rc or Arc type
// Or maybe keep as AsRef, because Deref will eventually allow
// An Arc to get to something that implements AsRef<[u8]>,
// Unsure of how this works actually

I agree we need to think about this to open the possibilities. Deref is just as limiting though. So perhaps we need a trait that we implement for common types

[josephDunne]

// We need to figure out a good way to expose methods like
// create_string on the WeaponBuilder itself.
//
// We could make a Builder trait that exposes these functions
// of the underlying Builder struct.

a trait for Builder and Table i think is the right way forward

[josephDunne]

// Impl for the WeaponBuilder
//
// The big difference here is that we create a new struct
// for a WeaponBuilder instead of implementing a trait
// on flatbuffers::Builder. This should reduce name collision
// possibilities and also be a bit cleaner to work with.

there shouldnt be any name collisions between the builder trait and the impl so think this is ok. Also the name collision will only happen if the ned user of WeaponBuilder imported the trait into scope. Normal usage would be just to import WeaponBuilder and use its impl API

[josephDunne]

Oh I think you where exploring the possibility of making each individual builder a trait itself. No I agree with you that thats not going to work well. Each builder instance will be a struct with normal member functions that utilize common builder functions under the hood (by implementing the common Builder trait).

[josephDunne]

// This is made possible by having the underlying builder be
// an Arcflatbuffers::Builder or some other shared pointer.
//

Builders are not Sync so would should avoid the Arc<Builder>. But we do need to think about how nested builders work. One option:

impl WeaponBuilder {
    fn add_meta_info(self) -> MetaInfoBuilder {
         MetaInfoBuilder::new_with(self)
   }

 //optimization:
   fn add_meta_info(self, history: String, age: u16) ->Self {
         MetaInfoBuilder::new_with(self)
                                      .add_history(history)
                                     .add_age(age)
                                    .build()
   }
}

then

impl MetaInfoBuilder
   fn build(self) -> T {
       ///where T in this instance is WeaponBuilder. But could also be Vec<u8> or &mut Vec<u8>
       /// or even SmallVec<u8>
      /// or even [u8;1500] etc
   }

So...it seems Builders need to be generic over some underlying buffer. They need to be able to build into any type that implements a vector like api. that could be a Vec or a &mut Vec or another Builder. As part of implementing the common Builder trait each builder instance will need to expose a vector like api

[josephDunne]

// A big difference in functionality between this setup
// and the current setup is that WeaponBuilder is no
// longer a trait that is implemented for flatbuffers::Builder,
// rather it is its own struct that references the flatbuffers::Builder.

I need to just double check here. I agree that WeaponBuilder should be a struct with functions as you have in your gist. I am happy to 'hide' the underlying builder as long as we are sure this is not restricting anyone. We could then implement an api that exposes the underlying builder for those that need it. Alternatively if instead of a hidden builder struct we could have a builder trait. That trait is imported into the module that implements WeaponBuilder and WeaponBuilder then implements what it needs to. In addition to implementing the Builder trait it exposes its own domain specific api i.e. add_name etc as normal method functions. The end user of WeaponBuilder need only import WeaponBuilder to use it. If the end user wants access to the underlying builder then they can import the flatbuffers::Builder trait into scope and use WeaponBuilder as a dumb builder so:

extern crate flatbuffers;
use flatbuffers::{Table, Buffer}; //rename table to flatbuffer?

[.. prelude]

struct WeaponBuilder<T>(T)

impl<T: VectorLikeThing> Builder for WeaponBuilder {
     //common builder functions have defaults so no need to re-implement them
 
    //need to expose the underlying vector of data - not sure what this api should look like
    // for now:
   fn as_vec(&mut self) -> &mut Vec<u8> {
       self.0.as_vec()
   }
}

pub fn new_weapon_builder() -> WeaponBuilder <Vec<u8>> {
    WeaponBuilder(Vec::new())
}

impl<T: VectorLikeThing> WeaponBuilder {
   fn new_with(inner: T) -> WeaponBuilder<T> {
        WeaponBuilder(inner)
   }

    fn add_name<T>(self, name: T) -> Self
        where T: AsRef<str> {
       self.add_string(name.as_ref().to_string())
   }

  fn add_damage(self, damage: u16) -> Self  {
       self.add_u16(damage)
   }

  fn add_meta_info(self) -> MetaInfoBuilder<Self> {
      MetaInfoBuilder::new_with(self)
  }

  fn build(self) -> T {
     self.close_object();
     self.0
  }
}

[josephDunne]

I had a thought VectorLikeThing is just the Builder Trait itself:

extern crate flatbuffers;
use flatbuffers::{Table, Buffer}; //rename table to flatbuffer?

[.. prelude]

struct WeaponBuilder<T>(T)

pub fn new_weapon_builder() -> WeaponBuilder <Vec<u8>> {
    WeaponBuilder(Vec::new())
}

impl AsMut<T> for WeaponBuilder<T> {
    fn as_mut(&mut self) -> &mut T {
        self.0
   }
}

impl<T: Builder > WeaponBuilder {
   fn new_with(inner: T) -> WeaponBuilder<T> {
        WeaponBuilder(inner)
   }

    fn add_name<T>(self, name: T) -> Self
        where T: AsRef<str> {
       self.add_string(name.as_ref().to_string())
   }

  fn add_damage(self, damage: u16) -> Self  {
       self.add_u16(damage)
   }

  fn add_meta_info(self) -> MetaInfoBuilder<Self> {
      MetaInfoBuilder::new_with(self)
  }

  fn add_meta_info_with(self, history: String, age: u16) -> Self {
      MetaInfoBuilder::new_with(self)
                                   .add_history(history)
                                   .add_age(age)
                                  .build()
  }

  fn build(self) -> T {
     self.close_object();
     self.0
  }
}

then in the flatbuffers crate we have:

impl Builder for Vec<u8> { .. }
impl<T: AsMut<R>, R: Builder> Builder for T { .. }

[josephDunne]

Ive created a new branch rust_support_triage please work there unless you have started on rust_support already (want to keep notifications off the main google repo until we are ready to merge)
Also you will need to sign a google CLA now. see the link on the main google repo pull request issue

[hollinwilkins]

@josephDunne I signed the Google CLA. Here are some thoughts on your feedback:

This would force the end user to remember to call the struct SomethingSpec. Also if you have an existing code base it would prevent you from just adding derive annotations to existing structs. So I'm against the idea.

Agree, let's stick with Weapon. The problem with this is then we need to call our actual FlatBuffer table something else, like WeaponFlatBuffer, or namespace it fb::Weapon.
I'm wondering what this should look like?

// Possible improvement, which would cause object
 // to be automatically closed on behalf of the user
//
// The idea here is to prevent misuse of the library
pub fn build_weapon<F>(&mut self, f: F) -> flatbuffers::UOffsetT
  where F: FnOnce() -> () {
    self.builder.start_object(2);
    f();
    self.builder.end_object()
  }

Okay, let's stick with the standard. I think C++ provides the Create methods to help with this issue I was solving for here.

the all builder methods would take self and return self versus &mut self

I like this, thanks for sending the above link on this subject.

 pub fn build(self) -> Vec<u8> {
    self.end_object();
    //return underlying buffer which could be a Vec<u8> if we own the buffer or a &mut Vec<u8> if we are being used to populate a vector owned by another builder
}

I think this build method should return the offset, which can be used for building vectors and nested objects later. Right?
Looking at the C++ implementation, this is how it works there.

struct MonsterBuilder {
  flatbuffers::FlatBufferBuilder &fbb_;
  flatbuffers::uoffset_t start_;
  void add_pos(const Vec3 *pos) { fbb_.AddStruct(4, pos); }
  void add_mana(int16_t mana) { fbb_.AddElement<int16_t>(6, mana, 150); }
  void add_hp(int16_t hp) { fbb_.AddElement<int16_t>(8, hp, 100); }
  void add_name(flatbuffers::Offset<flatbuffers::String> name) { fbb_.AddOffset(10, name); }
  void add_inventory(flatbuffers::Offset<flatbuffers::Vector<uint8_t>> inventory) { fbb_.AddOffset(14, inventory); }
  void add_color(int8_t color) { fbb_.AddElement<int8_t>(16, color, 2); }
  MonsterBuilder(flatbuffers::FlatBufferBuilder &_fbb) : fbb_(_fbb) { start_ = fbb_.StartTable(); }
  flatbuffers::Offset<Monster> Finish() { return flatbuffers::Offset<Monster>(fbb_.EndTable(start_, 7)); }
};

a trait for Builder and Table i think is the right way forward

I think we should follow the C++ implementation here and have a reference to an underlying builder:
A trait for Table is probably useful, we should define what functionality it would offer.

Users can always access the builder through an accessor function or with the original builder that was created for the table.

Builders are not Sync so would should avoid the Arc. But we do need to think about how nested builders work. One option:

Agreed, builders should not be Sync, we should make sure the Send is not implemented for Builder.

I had a thought VectorLikeThing is just the Builder Trait itself:

I think that there is a lot more to builders than just a vector (at least in the current implementation). There is a lot of information stored
about the vtable and some other record keeping items. I'll look more into the C++ implementation to see how they handle these things.

Briefly looking at the current implementation of builder, there are many fields:

pub struct Builder {
    // Where the FlatBuffer is constructed.
    bytes: Vec<u8>,
    // Minimum alignment encountered so far.
    min_align: usize,
    // Starting offset of the current struct/table.
    object_start: UOffsetT,
    // Whether we are currently serializing a table.
    nested: bool,
    // Whether the buffer is finished.
    finished: bool,
    // The vtable for the current table.
    vtable: Vec<UOffsetT>,
    // The number of vtable slots required by the current object.
    vtable_in_use: usize,
    // Offsets of other Vtables in the buffer.
    vtables: Vec<UOffsetT>,
    // Unused space in the buffer. Size from beggining of buffer to first vtable
    space: usize,
    // Number of elements in the current vector
    vector_len: usize,
    // If false builder will ignore default values and always create a field
    // and non-zero vslot. Default is true and will only create an entry
    // if value != default.
    force_defaults: bool,
    // A flag to control the vtable deduplication process. see `write_vtable`
    vtable_dedup: bool,
}

WeaponBuilder should just reference a Builder, and any other derived builders (Monster, Vec3, etc.) can all reference the same underlying Builder struct. If we make a Builder !Send, then we don't have to worry about multiple threads accessing it at the same time. We could use an Rc in this case for the underlying data.

I agree we need to think about this to open the possibilities. Deref is just as limiting though. So perhaps we need a trait that we implement for common types

I think you were correct in using AsRef to begin with, because I think pointer-like objects like an Rc will, through the compiler following a Deref chain, be able to call AsRef<[u8]>. I can verify this, but seems likely how it works.

[josephDunne]

I think we should follow the C++ implementation here and have a reference to an underlying builder

Ok agreed. My only concern is that this UOffset is actually a reference to an offset in a buffer that has some lifetime and the Rust type system could express that if we wanted. I will play around and see.

Agreed, builders should not be Sync, we should make sure the Send is not implemented for Builder

They can be Send - for example you might have a pipeline of threads each appending to a builder one after the other. The only issue is controlling access to the underlying buffer. Only one builder at a time can have access to the &mut [u8]

If we make a Builder !Send, then we don't have to worry about multiple threads accessing it at the same time.

!Sync will stop threads from sharing one reference to the same builder. But one builder can still be sent across threads (you cant send references across thread boundaries unless you wrap it up in a struct and that struct will then take ownership of that one builder. !Sync will stop that struct from being cloned etc)

Briefly looking at the current implementation of builder, there are many fields:

I forgot about this. So you are right to have a builder reference another builder struct.

[josephDunne]

I think we should follow the C++ implementation here and have a reference to an underlying builder

Ok agreed. My only concern is that this UOffset is actually a reference to an offset in a buffer that has some lifetime and the Rust type system could express that if we wanted. I will play around and see.

I tack this back. The builder owns the vec so it cant return just a UOffset it has to return the vec at some point:

pub struct Builder {
    // Where the FlatBuffer is constructed.
    bytes: Vec<u8>,
...

[hollinwilkins]

I tack this back. The builder owns the vec so it cant return just a UOffset it has to return the vec at some point:

We should share Builder between WeaponBuilder, MonsterBuilder, etc. as some sort of pointer. My initial thought is an Arc<Builder>, but we could also make it more generic with AsMut<Builder>. This would allow us to return the offset and we can match C++ in this regard.

Thinking about threads, concurrency, etc. I don't think we should be concerned too much with it anymore. The implementor can choose the underlying AsMut<Builder> that is powering everything. If they decide it should be an ReadWriteLock<Arc<Builder>>, then they should know all of the concerns that come with that and deal with it in application logic.

Very roughly, we could structure it like this:

pub struct FlatBufferBuilder {
  buf: DownVec, // based on vector_downward from C++ code, Vec<u8> as base buffer
  ... all of the other fields
}

impl FlatBufferBuilder {
  fn get_mut_buf(&mut self) -> &mut Vec<u8>;
  fn get_buf(&mut self) -> &Vec<u8>;
  fn to_owned_buf(self) -> Vec<u8>;

  fn get_force_defaults(&self) -> bool;
  fn get_dedup_vtables(&self) -> bool;
  fn get_pad(&self) -> usize;
  fn get_align(&self) -> usize;

  fn force_defaults(self, force_defaults: bool) -> Self;
  fn dedup_vtables(self, dedup_vtables: bool) -> Self;
  fn pad(self, pad: usize) -> Self;
  fn align(self, align: usize) -> Self;

  ... implementations for all the builder functions
}

// Base trait for all implementors of a table builder
// We use methods defined on the trait so that we don't
// ever collide with names defined on the table itself
pub trait TableBuilder {
  fn get_mut_builder(b: &mut Self) -> &mut FlatBufferBuilder;
  fn get_builder(b: &Self) -> &FlatBufferBuilder;
}

pub trait WeaponBuilder: TableBuilder {
  .. default implementations
}

// Define our provided structs here that can be used as weapon builders
// out of the box, this still allows users to implement their own weapon builders
// if need be
pub struct SharedWeaponBuilder {
  builder: Arc<FlatBufferBuilder>
}

pub struct RcWeaponBuilder {
  builder: Rc<FlatBufferBuilder>
}

// Define all implementations here
impl TableBuilder for SharedWeaponBuilder {
  fn get_mut_builder(b: &mut Self) -> &mut FlatBufferBuilder { self.builder.as_mut_ref() }
  fn get_builder(b: &Self) -> &FlatBufferBuilder { self.builder.as_ref() }
}
impl WeaponBuilder for SharedWeaponBuilder { }

impl TableBuilder for RcWeaponBuilder {
  fn get_mut_builder(b: &mut Self) -> &mut FlatBufferBuilder { self.builder.as_mut_ref() }
  fn get_builder(b: &Self) -> &FlatBufferBuilder { self.builder.as_ref() }
}
import WeaponBuilder for RcWeaponBuilder { }

[josephDunne]

We should share Builder between WeaponBuilder, MonsterBuilder, etc. as some sort of pointer.

This is where we disagree. The introduction of a pointer is extra complication that I don't think fits with the flatbuffer idea. My reasoning: A builder builds a owned slice of [u8] and builds in an append only manner. So there is only ever one buffer and one builder doing the appending. So builders should take ownership of the underlying buffer/sub builder. Introducing Arc/Rc/references inside this crate is I think mixing in the wrong ideas. Here is the primary use case:

WeaponBuilder::new()
    .add_name("Doom")
    .add_damage(16)
     .add_meta_info()   //this returns a MetaInfoBuilder that now owns the underlying buffer (in this scenario the Buffer is WeaponBuilder
          .add_history("This sword was forged in the fires of Hell")
          .build() // completes the meta info buffer and returns /WeaponBuilder
     .build() // return the vec<u8> to be saved to disk or sent over wire

All data is owned and there are no references so this structure is 'static and therefore safe to implement Send - a new thread could take ownership and complete add its own stuff to the buffer if it wanted. It takes ownership though there is no Sync access

If you do have a concurrent builder (as a secondary use case) then you need to introduce the concept of synchronise access and the only way that is possible is as you had above with a read write lock: Arc<Mutex<Builder>>.
So here again once a thread has acquired the mutex it has ownership (for the duration of the MutexGaurd. No sharing of references.

So the append only nature of the builder suggests to me that ownership not shared references are the way forward. Here is how we could implement the above code snippet:

pub trait Builder {
    type Buffer;
    fn add_u8(self, data: u8) -> Self;
    fn add_string<T: AsRef<Str>(self, data: T) -> Self;
    fn build() -> Self::Buffer;
}

pub struct BufferBuilder {
    // lots of fields here
   buffer: Vec<u8>
}

impl Builder for BufferBuilder {
   type Buffer = Vec<u8>;

    fn add_u8(mut self, data: u8) {
         self.buffer.push(data);
         self
    }
    fn add_string<T: AsRef<Str>(mut self, data: T) {
       self..buffer.extend_from_slice(data.as_ref().as_bytes());
       self
    }

   fn builder(self) -> Vec<u8> {
       //finish what ever needs to be done and then return
       self.buffer
   }
}

impl From<Vec<u8>> for BufferBuilder {
..
}


pub struct WeaponBuilder<T> {
     // some fields here relating to weapons?
      buffer: T
}

// Repition here. Will be easier in future releases of Rust where we can delegate the trait impl to 
// T
impl<T: Builder> Builder for WeaponBuilder<T> {
    fn add_u8(mut self, data: u8) {
         self.buffer.add_u8(data)
    }
    fn add_string<T: AsRef<Str>(mut self, data: T) {
       self.buffer.add_string(data)
    }

   fn builder(self) -> Self::Buffer {
       //finish what ever needs to be done and then return
       self.buffer
   }
}

impl<T: Builder> WeaponBuilder<T> {
    pub fn new() -> Self {
          let builder: BufferBuilder = Vec::with_capacity(blah).into();
          WeaponBuilder {
               buffer: builder
          }
    }

    pub fn add_damage(mut self, damage: u8) -> Self {
        self.buffer.add_u8(damage)
    } 

    pub fn add_name<S: AsRef<Str>>(mut self, name: S) -> Self {
        self.buffer.add_string(name)
    }

    pub fn add_meta_info(Self) -> MetaInfoBuilder {
        let builder =  MetaInfoBuilder {
              buffer: self
         };
         //builder.start_struct()
         builder
    }

}


pub struct MetaInfoBuilder<T> {
     // some fields here relating to weapons?
      buffer: T
}

// Repition here. Will be easier in future releases of Rust where we can delegate the trait impl to 
// T
impl<T: Builder> Builder for MetaInfoBuilder <T> {
    fn add_u8(mut self, data: u8) {
         self.buffer. add_u8(data)
    }
    fn add_string<T: AsRef<Str>(mut self, data: T) {
       self.buffer.add_string(data)
    }

   fn builder(self) -> Self::Buffer {
       //finish what ever needs to be done and then return
       self.buffer
   }
}

impl<T: Builder> MetaInfoBuilder <T> {
    pub fn new() -> Self {
          let builder: BufferBuilder = Vec::with_capacity(blah).into();
          MetaInfoBuilder {
               buffer: builder
          }
    }

    pub fn add_history<S: AsRef<Str>>(mut self, name: S) -> Self {
        self.buffer.add_string(name)
    }
}

There are gaps in the above implementation. As you pointed out builders have a lot of state and the exact sequence of calls need to be worked out. IF references are necessary then the only one that makes sense is a &mut Builder (or perhaps more simply: &mut Vec<u8>) and then you have to start putting lifetime parameters and rely on the rust borrow checker to make sure only one builder is ever appending to the buffer. I don't think this is a use case for Rc or Arc. I think end users of this crate will often wrap flatbuffers in an Rc or Arc but a flatbuffer is read only so that makes more sense

[hollinwilkins]

@josephDunne I agree with you and see what you're going for here. I'm going to come up with some skeleton implementations and post them here.

Agreed adding Rc/Arc is heavyweight, and relying on Rust borrow checker to ensure just one builder at a time would be a huge win. I'll write some code and get back to you with something here.

[hollinwilkins]

use types::*;
use std::marker::PhantomData;

// DSL example of what we can do with this
// code skeleton structure
pub fn test_dsl() {
  let mut builder = FlatBufferBuilder::new();

  // Create all of the strings we are going to use here
  let weapon_name = builder.create_string("Vicious Weapon!");
  let meta_key = builder.create_string("meta_key");
  let meta_value = builder.create_string("meta_value");
  let meta_key1 = builder.create_string("meta_key1");
  let meta_value1 = builder.create_string("meta_value1");
  let meta_key2 = builder.create_string("meta_key2");
  let meta_value2 = builder.create_string("meta_value2");

  // FlatBufferBuilder is consumed when creating
  // a table builder
  {
    let wb = WeaponBuilder::new(&mut builder);
    wb.add_name(weapon_name).
      add_damage(23).
      add_meta(|mb| {
        mb.add_key(meta_key).
          add_value(meta_value).
          build()
      }).add_meta_vector(|mvb| {
        // Use add_next to add elements to a vector
        // All tracking for offsets is done in
        // The VectorBuilder
        mvb.add_next(|mb| {
          mb.add_key(meta_key1).
            add_value(meta_value1).
            build()
        }).add_next(|mb| {
          mb.add_key(meta_key2).
            add_value(meta_value2).
            build()
        }).build()
      }).build();
  }
  let mybuf = builder.to_owned_buf();
}

pub struct Offset<T> {
  offset: UOffsetT,
  _pd: PhantomData<* const T>
}

impl<T> Offset<T> {
  pub fn new(offset: UOffsetT) -> Offset<T> {
    Offset {
      offset: offset,
      _pd: PhantomData
    }
  }
}

// Like a Vec<u8>, but grows backwards and elements
// get appended to the front instead of the back
//
// This will be a minimal API for us to use, as is
// done in the C++ implementation
pub struct VecDownward {
  // Other fields needed for a downward vec
  buf: Vec<u8>
}

impl VecDownward {
  pub fn new() -> VecDownward {
    VecDownward {
      buf: Vec::new()
    }
  }

  pub fn to_owned_buf(self) -> Vec<u8> { self.buf }
}

// FlatBufferBuilder owns the underlying buffer
pub struct FlatBufferBuilder {
  // VecDownward is the backing buffer type
  buf: VecDownward,
  //nested: bool,
  //finished: bool,
  //minalign: usize,
  //force_defaults: bool,
  //dedup_vtables: bool,
  //offsets: Vec<FieldLoc>,
  //string_pool: Map<FlatString, Offset<FlatString>>
}

impl FlatBufferBuilder {
  pub fn new() -> FlatBufferBuilder {
    FlatBufferBuilder {
      buf: VecDownward::new()
    }
  }

  pub fn create_string(&mut self, v: &str) -> Offset<String> {
    Offset::new(42)
  }

  pub fn to_owned_buf(self) -> Vec<u8> {
    self.buf.to_owned_buf()
  }
}

pub trait TableBuilder<'a> {
  // All TableBuilders take a FlatBufferBuilder for new
  fn new(builder: &'a mut FlatBufferBuilder) -> Self;

  // Done building, release ownership of the FlatBufferBuilder
  fn build(self) -> &'a mut FlatBufferBuilder;
}

pub struct MetaBuilder<'a> {
  builder: &'a mut FlatBufferBuilder
}

impl<'a> TableBuilder<'a> for MetaBuilder<'a> {
  fn new(builder: &'a mut FlatBufferBuilder) -> MetaBuilder<'a> {
    MetaBuilder {
      builder: builder
    }
  }

  fn build(self) -> &'a mut FlatBufferBuilder { self.builder }
}

impl<'a> MetaBuilder<'a> {
  pub fn add_key(self, v: Offset<String>) -> Self { self }
  pub fn add_value(self, v: Offset<String>) -> Self { self }
}

pub struct VectorBuilder<'a, B: TableBuilder<'a> + 'a> {
  builder: &'a mut FlatBufferBuilder,
  // other data needed to store vector, like offsets
  table_builder: PhantomData<&'a B>
}

impl<'a, B: TableBuilder<'a>> VectorBuilder<'a, B> {
  pub fn new(builder: &'a mut FlatBufferBuilder) -> VectorBuilder<'a, B> {
    VectorBuilder {
      builder: builder,
      table_builder: PhantomData
    }
  }

  pub fn add_next<F>(self, f: F) -> Self where
    F: FnOnce(B) -> &'a mut FlatBufferBuilder {
      let b = B::new(self.builder);
      let builder = f(b);
      Self::new(builder)
    }

  fn build(self) -> &'a mut FlatBufferBuilder {
    // Finalize the vector information
    self.builder
  }
}

pub struct WeaponBuilder<'a> {
  builder: &'a mut FlatBufferBuilder
}

impl<'a> TableBuilder<'a> for WeaponBuilder<'a> {
  fn new(builder: &'a mut FlatBufferBuilder) -> WeaponBuilder<'a> {
    WeaponBuilder {
      builder: builder
    }
  }

  fn build(self) -> &'a mut FlatBufferBuilder { self.builder }
}

impl<'a> WeaponBuilder<'a> {
  pub fn add_name(self, v: Offset<String>) -> Self { self }
  pub fn add_damage(self, v: u16) -> Self { self }
  pub fn add_meta<F>(self, f: F) -> Self where
    F: FnOnce(MetaBuilder<'a>) -> &'a mut FlatBufferBuilder {
      let mb = MetaBuilder::new(self.builder);
      let builder = f(mb);
      Self::new(builder)
    }
  pub fn add_meta_vector<F>(self, f: F) -> Self where
    F: FnOnce(VectorBuilder<'a, MetaBuilder<'a>>) -> &'a mut FlatBufferBuilder {
      let vab: VectorBuilder<'a, MetaBuilder<'a>> = VectorBuilder::new(self.builder);
      let builder = f(vab);
      Self::new(builder)
    }
}

[hollinwilkins]

Let me know what you think of the above. I tried to take into account all of the things we've talked about. I like the DSL a lot, lots of implementation details missing obviously, but I don't think any individual piece will be too difficult.

[josephDunne]

I say give the above a go. I can't see anything wrong. I don't like passing in closures but if it works it works.

[hollinwilkins]

@josephDunne Sounds good, I will go ahead with this design then. If we decide we want to get rid of the closure DSL in the future, should be a pretty easy change.